option for a seperate audit log file by blizzz · Pull Request #9293 · nextcloud/server

blizzz · 2018-04-24T20:15:37Z

log classes are not static anymore
apps can have their own log file with code reuse
admin audit makes use of it
check rotating magic
logreader reads old, static, private classes 😮 → adjust to changes from server/#9293 logreader#71
log to seperate file by default
~~evaluate gui for changing log location for admin_audit~~ we don't have this for the general log file either, can be controlled via config:app:set admin_audit logfile --value="/var/log/nextcloud/audit.log" rotation file size follows general setting

codecov · 2018-04-24T20:23:53Z

Codecov Report

Merging #9293 into master will increase coverage by <.01%.
The diff coverage is 32.12%.

@@             Coverage Diff              @@
##             master    #9293      +/-   ##
============================================
+ Coverage     51.93%   51.93%   +<.01%     
- Complexity    25388    25402      +14     
============================================
  Files          1608     1609       +1     
  Lines         95429    95470      +41     
  Branches       1394     1394              
============================================
+ Hits          49559    49584      +25     
- Misses        45870    45886      +16

Impacted Files	Coverage Δ	Complexity Δ
apps/federation/lib/SyncJob.php	`33.33% <0%> (ø)`	`3 <0> (ø)`	⬇️
apps/files_trashbin/lib/Storage.php	`76.72% <0%> (ø)`	`35 <0> (ø)`	⬇️
lib/private/legacy/app.php	`62.63% <0%> (ø)`	`196 <0> (ø)`	⬇️
...e/AppFramework/DependencyInjection/DIContainer.php	`82.8% <0%> (ø)`	`54 <0> (ø)`	⬇️
lib/private/User/Database.php	`79.47% <0%> (ø)`	`43 <0> (ø)`	⬇️
lib/public/Util.php	`60.19% <0%> (ø)`	`45 <1> (ø)`	⬇️
lib/private/Preview/SVG.php	`56% <0%> (ø)`	`6 <0> (ø)`	⬇️
settings/ajax/enableapp.php	`0% <0%> (ø)`	`0 <0> (ø)`	⬇️
lib/private/Setup/MySQL.php	`0% <0%> (ø)`	`4 <0> (ø)`	⬇️
lib/private/Files/Storage/Wrapper/Encryption.php	`71.22% <0%> (ø)`	`143 <0> (ø)`	⬇️
... and 67 more

nickvergessen · 2018-04-25T10:23:17Z

 		// FIXME: Add this for backwards compatibility, should be fixed at some point probably
 		if ($config === null) {
-			$config = \OC::$server->getSystemConfig();
+			$config = \OC::$server->getConfig();


This breaks logging while installing?!

Fairly sure switching from systemconfig to config will break yes

nickvergessen · 2018-04-25T10:24:12Z

 		}

-		return min($this->config->getValue('loglevel', Util::WARN), Util::FATAL);
+		return min($this->config->getSystemValue('loglevel', Util::WARN), Util::FATAL);


Should move those consts to ILogger?

icewind1991 · 2018-04-25T11:04:24Z

+use OCP\Log\IWriter;
+
+class Syslog implements IWriter {
+	static protected $levels = [


you can use class constant arrays with the currently supported php versions for a bit cleaner code

missed that one, thanks for the hint. What do you have in mind with class constants however?

icewind1991 · 2018-04-25T11:05:20Z

-		// Close at shutdown
+	public function __construct(IConfig $config) {
+		openlog($config->getSystemValue('syslog_tag', 'ownCloud'), LOG_PID | LOG_CONS, LOG_USER);
 		register_shutdown_function('closelog');


do this with a destructor now?

icewind1991 · 2018-04-25T11:25:18Z

 		// FIXME: Add this for backwards compatibility, should be fixed at some point probably
 		if ($config === null) {
-			$config = \OC::$server->getSystemConfig();
+			$config = \OC::$server->getConfig();


Fairly sure switching from systemconfig to config will break yes

blizzz · 2018-04-25T13:35:37Z

✓ reverted change to IConfig in Log
✓ moved Log constants to ILogger and depracted them in Util
✓ polished Syslog a bit

codecov · 2018-04-25T15:05:35Z

Codecov Report

Merging #9293 into master will increase coverage by <.01%.
The diff coverage is 46.66%.

@@             Coverage Diff              @@
##             master    #9293      +/-   ##
============================================
+ Coverage     51.93%   51.93%   +<.01%     
- Complexity    25388    25407      +19     
============================================
  Files          1608     1611       +3     
  Lines         95440    95484      +44     
  Branches       1394     1394              
============================================
+ Hits          49564    49588      +24     
- Misses        45876    45896      +20

Impacted Files	Coverage Δ	Complexity Δ
.../admin_audit/composer/composer/autoload_static.php	`0% <ø> (ø)`	`1 <0> (ø)`	⬇️
...dmin_audit/composer/composer/autoload_classmap.php	`0% <0%> (ø)`	`0 <0> (ø)`	⬇️
lib/private/Log/Errorlog.php	`0% <0%> (ø)`	`1 <1> (-1)`	⬇️
lib/public/Log/RotationTrait.php	`0% <0%> (ø)`	`4 <4> (?)`
lib/private/Log/Rotate.php	`0% <0%> (ø)`	`2 <0> (-2)`	⬇️
lib/private/Log/Syslog.php	`0% <0%> (ø)`	`3 <3> (+1)`	⬆️
lib/private/Log/LogFactory.php	`100% <100%> (ø)`	`11 <11> (?)`
apps/admin_audit/lib/BackgroundJobs/Rotate.php	`18.18% <18.18%> (ø)`	`4 <4> (?)`
apps/admin_audit/lib/AppInfo/Application.php	`13.57% <26.53%> (+5.17%)`	`22 <18> (+2)`	⬆️
lib/private/Log.php	`73.07% <50%> (-4.46%)`	`35 <5> (-5)`
... and 7 more

MorrisJobke · 2018-04-26T08:48:58Z

✓ moved Log constants to ILogger and depracted them in Util

Moved to a separate PR: #9308

MorrisJobke · 2018-04-26T09:55:23Z

Moved to a separate PR: #9308

Let me rebase and fix the conflicts in this PR as the other one was merged.

Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>

rullzer · 2018-04-26T20:21:27Z

 	 * @param int $level
 	 */
-	public static function write($app, $message, $level) {
+	public function write(string $app, $message, int $level) {


string $message?

File takes arrays when logging exceptions :( and this needs to match the Interface

fair enough

rullzer · 2018-04-26T20:24:20Z

+		$this->registerService(ILogFactory::class, function (Server $c) {
+			return new LogFactory($c);
+		});
+		$this->registerAlias('LogFactory', ILogFactory::class);


rullzer · 2018-04-26T20:24:57Z

+	 * @throws \OCP\AppFramework\QueryException
+	 */
+	public function getLogFactory() {
+		return $this->query('LogFactory');


You can just query for ILogFactory::class

Or you remove this function in general and just query for the class where you need it

rullzer · 2018-04-26T20:27:11Z

-				if(!touch(self::$logFile)) {
-					self::$logFile = $defaultLogFile;
-				}
+	public function __construct(string $path, string $fallbackPath = '', IConfig $config) {


Any reason to require IConfig over SystemConfig here? It is internal. And the systemconfig doesn't need DB stuff etc. So less change of cyclic dependencies.

Probably i had in mind back then making to adding API for it, but went another path… anyway, using SystemConfig makes sense. I'll adapt.

Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>

Fixed.

MorrisJobke

Tested and works 👍

eniac111 · 2018-08-24T14:09:09Z

Hey, do you have any documentation on this?

blizzz · 2018-08-24T14:59:40Z

@eniac111 in the description ↑ 🙊

config:app:set admin_audit logfile --value="/var/log/nextcloud/audit.log"

MorrisJobke · 2018-08-24T15:36:28Z

@eniac111 in the description ↑ 🙊

Could you add it to the documentation/manual?

j-ed · 2019-03-12T12:26:52Z

@MorrisJobke I just stumbled over the missing documentation of the audit log function too. To be able to improve the documentation I would need some more information about this function, which is enabled by activating the admin_audit app.
I wonder if it is correct that only audit information (logins/logoffs) is being logged or if other applications should use it too (I also saw "trashbin:expire" log entries in this file)?

MorrisJobke · 2019-03-12T12:35:44Z

@MorrisJobke I just stumbled over the missing documentation of the audit log function too. To be able to improve the documentation I would need some more information about this function, which is enabled by activating the admin_audit app.

This separate log file is already used, but with this there is a way to customize the path of this file via:

config:app:set admin_audit logfile --value="/var/log/nextcloud/audit.log"

I wonder if it is correct that only audit information (logins/logoffs) is being logged or if other applications should use it too (I also saw "trashbin:expire" log entries in this file)?

Only admin_audit entries are logged and we should not use settings of apps in other apps. So this then needs to be implemented in the trash bin as well.

j-ed · 2019-03-12T14:59:05Z

@MorrisJobke I've createds the following pull request for the documentation change: nextcloud/documentation#1300

Do I understand you correct that it is an error that "trashbin:expire" messages are logged in the audit.log? If yes, I would create a separate issue ticket to get that fixed:

{"reqId":"1qxu8bvbtOn53RyaB3S4","level":1,"time":"2019-03-12 15:56:04+01:00","remoteAddr":"","user":"--","app":"admin_audit","method":"","url":"\/nextcloud\/occ","message":"Console command executed: trashbin:expire --quiet","userAgent":"--","version":"15.0.5.3"}

nickvergessen · 2019-03-12T15:10:44Z

Do I understand you correct that it is an error that "trashbin:expire" messages are logged in the audit.log? If yes, I would create a separate issue ticket to get that fixed:

All occ calls are logged to the auditing and have to be. Since they allowed to disabled apps (e.g. the logging app) it would be crucial to not log that.

blizzz added enhancement 2. developing Work in progress feature: admin audit labels Apr 24, 2018

blizzz added this to the Nextcloud 14 milestone Apr 24, 2018

blizzz force-pushed the feature/9166/custom-auditlogfile branch from a745c66 to 011613d Compare April 24, 2018 20:23

blizzz force-pushed the feature/9166/custom-auditlogfile branch 9 times, most recently from a4e55e2 to 07fd13f Compare April 25, 2018 09:55

nickvergessen reviewed Apr 25, 2018

View reviewed changes

blizzz mentioned this pull request Apr 25, 2018

adjust to changes from server/#9293 nextcloud/logreader#71

Merged

1 task

icewind1991 previously requested changes Apr 25, 2018

View reviewed changes

blizzz force-pushed the feature/9166/custom-auditlogfile branch from 4f4ac38 to 6347106 Compare April 25, 2018 14:12

blizzz added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Apr 25, 2018

blizzz requested review from MorrisJobke, icewind1991, nickvergessen and rullzer April 25, 2018 15:46

blizzz force-pushed the feature/9166/custom-auditlogfile branch from 71bbf60 to 4c7fd14 Compare April 25, 2018 21:36

destaticfy Log classes

5fbf184

Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>

MorrisJobke force-pushed the feature/9166/custom-auditlogfile branch from 4c7fd14 to 90e1394 Compare April 26, 2018 10:05

blizzz added 7 commits April 26, 2018 12:10

offer API to create own File log. admin_audit makes use of it

cfc3ab0

Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>

move IFileBased to public namespace, logreader needs it

6959240

Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>

revert Log's dependency to SystemConfig to work during Installation

0e6a317

Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>

improve Syslog a little

a21a5bc

Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>

add missing php doc and type hints

5751788

Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>

fix dav test

ab7a4b8

Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>

log to $datadir/audit.log by default and add rotation

b841a47

Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>

MorrisJobke force-pushed the feature/9166/custom-auditlogfile branch from 90e1394 to b841a47 Compare April 26, 2018 10:17

rullzer requested changes Apr 26, 2018

View reviewed changes

use SystemConfig, less dependencies, and not publicly needed

aff5fe6

Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>

rullzer approved these changes May 1, 2018

View reviewed changes

MorrisJobke approved these changes May 2, 2018

View reviewed changes

MorrisJobke merged commit 2aa1080 into master May 2, 2018

MorrisJobke deleted the feature/9166/custom-auditlogfile branch May 2, 2018 14:16

MorrisJobke mentioned this pull request May 2, 2018

Seperate Audit Log file #9166

Closed

4 tasks

Uh oh!

Uh oh!

Conversation

blizzz commented Apr 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov Bot commented Apr 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

blizzz Apr 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

blizzz commented Apr 25, 2018

Uh oh!

codecov Bot commented Apr 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

MorrisJobke commented Apr 26, 2018

Uh oh!

MorrisJobke commented Apr 26, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

blizzz Apr 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MorrisJobke left a comment

Choose a reason for hiding this comment

Uh oh!

eniac111 commented Aug 24, 2018

Uh oh!

blizzz commented Aug 24, 2018

Uh oh!

MorrisJobke commented Aug 24, 2018

Uh oh!

j-ed commented Mar 12, 2019

Uh oh!

MorrisJobke commented Mar 12, 2019

Uh oh!

j-ed commented Mar 12, 2019

Uh oh!

nickvergessen commented Mar 12, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

blizzz commented Apr 24, 2018 •

edited

Loading

codecov Bot commented Apr 24, 2018 •

edited

Loading

blizzz Apr 25, 2018 •

edited

Loading

codecov Bot commented Apr 25, 2018 •

edited

Loading

blizzz Apr 26, 2018 •

edited

Loading